Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix to enable links in markdown #3481

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix to enable links in markdown #3481

wants to merge 1 commit into from

Conversation

mahibi
Copy link
Collaborator

@mahibi mahibi commented Nov 30, 2023 •
edited
Loading

fix #3633

fix to enable links in markdown when no top level domain was included in the link description.

Examples (without TLD):

Hacker News
AlternativeTo
Phoronix
BNN-Breaking
OSBA – Open Source Business Alliance

Examples (with TLD):

LinuxNews.de

🖼️ Screenshots

🏚️ Before 🏡 After
Bildschirmfoto vom 2023-11-30 11-02-24 geaendert Bildschirmfoto vom 2023-11-30 11-02-33 geaendert

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

@mahibi mahibi added the 3. to review Waiting for reviews label Nov 30, 2023
@mahibi mahibi added this to the 18.1.0 milestone Nov 30, 2023
@mahibi mahibi self-assigned this Nov 30, 2023
@mahibi
Copy link
Collaborator Author

mahibi commented Nov 30, 2023

/backport to stable-18.0

@mahibi mahibi added 1. to develop Accepted and waiting to be taken care of (should be only set by nextcloud employees) and removed 3. to review Waiting for reviews labels Nov 30, 2023
@mahibi
Copy link
Collaborator Author

mahibi commented Nov 30, 2023 •
edited
Loading

ah this destroys other links, e.g. "mailto:" and phone numbers.

As soon as
app:textAutoLink is set to some value, markdown links with no top level domain in the link description fail to be shown as links.

Also
app:textAutoLink="email|phone|map|web"
or
app:textAutoLink="email|phone|map"
won't help.

Will have a closer look later..

@mahibi mahibi added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of (should be only set by nextcloud employees) labels Nov 30, 2023
@mahibi mahibi marked this pull request as draft November 30, 2023 10:20
@mahibi mahibi mentioned this pull request Feb 12, 2024
Open
@mahibi
Copy link
Collaborator Author

mahibi commented Feb 12, 2024

when work on this PR is continued, also have a look at #3634

@mahibi mahibi modified the milestones: 18.1.0, 19.0.0 Mar 12, 2024
@mahibi mahibi removed this from the 19.0.0 milestone Apr 23, 2024
@mahibi mahibi force-pushed the bugfix/noid/fixMarkdownLinks branch from 8adf18b to fe18bb4 Compare May 7, 2024 18:21
@mahibi
Copy link
Collaborator Author

mahibi commented May 7, 2024 •
edited
Loading

So on one hand there is the need to support all markdown links, on the other hand there is automatic parsing.

It seems for now there is only the decision to enable or disable app:textAutoLink="all". If there are other solutions they might require quite some work.
Also other messengers handle this quite differently and none of them supports all options. Especially with markdown (which most other messengers don't support) things seem to get more complicated.

Option1: with app:textAutoLink="all" Option2: without app:textAutoLink="all"
Markdown links are broken (Only when TLD is in the description, it's recognized as hyperlink. But the actual link from markdown might be different and is not taken into account). All other content is clickable if patterns match, but could also be senseless (esp. for phonenumbers) All markdown links are working as expected. Nothing else is recognized as clickable content (but link previews still work if a link was found)
grafik grafik

I personally vote to go with option2, so markdown links are working and all other content can be copied manually if needed.

@mahibi @AndyScherzinger
fix to enable links in markdown
cabe162 
fix to enable links in markdown when no top level domain was included in the link description

This will disable automatic link parsing for hyperlinks, numbers, email address,...

Signed-off-by: Marcel Hibbe <[email protected]>
@AndyScherzinger AndyScherzinger force-pushed the bugfix/noid/fixMarkdownLinks branch from fe18bb4 to cabe162 Compare July 9, 2024 21:03
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Codacy

Lint

TypemasterPR
Warnings9090
Errors129129

SpotBugs

CategoryBaseNew
Bad practice66
Correctness1111
Dodgy code8383
Internationalization33
Malicious code vulnerability33
Performance66
Security11
Total113113

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 2024

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/3481-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

@mahibi mahibi mentioned this pull request Oct 2, 2024
Draft
6 tasks
@mahibi
Copy link
Collaborator Author

mahibi commented Oct 2, 2024

This PR is also related to #4297 because app:textAutoLink="all" may be necessary.

@mahibi
Copy link
Collaborator Author

mahibi commented Oct 2, 2024 •
edited
Loading

Message from screenshots that that is used for testing (switch to edit view for copy pasting into chat):

markdown links:

AlternativeTo

LinuxNews.de

website as plain text:

https://heise.de

numbers as plain text:

123

1234

12345

123456

email address as plain text:

[email protected]

Koordinaten

52.57731/13.28111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mahibi mahibi

Labels
2. developing Work in progress backport-request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Markdown in message body not rendered
1 participant
@mahibi